Conversation
EmilianoSanchez
left a comment
There was a problem hiding this comment.
LGTM, but there are some minor comments to check
| :type operation: str | ||
| """ | ||
| try: | ||
| if method_name is not None: |
There was a problem hiding this comment.
do you need two params (method_name and operation), or can it be done with only one?
E.g.:
if operation is not None:
await self._telemetry_evaluation_producer.record_latency(operation, latency)
There was a problem hiding this comment.
method_name is optional from the original code, but operation is required. So the if has to move to the previous call.
This is the original code BTW.
| await self._telemetry_evaluation_producer.record_latency(operation, latency) | ||
| impressions = self._impressions_manager.process_impressions(impressions) | ||
| await self._impression_storage.put(impressions) | ||
| except Exception: # pylint: disable=broad-except |
There was a problem hiding this comment.
should you handle exception here or not, like in record_treatment_stats?
There was a problem hiding this comment.
Did you mean record_track_stats call? this call returns True or False only, the called function does have try/catch block in case there is an exception.
There was a problem hiding this comment.
Sorry, I meant record_track_stats
| await self._telemetry_evaluation_producer.record_latency(operation, latency) | ||
| impressions = self._impressions_manager.process_impressions(impressions) | ||
| await self._impression_storage.put(impressions) | ||
| except Exception: # pylint: disable=broad-except |
There was a problem hiding this comment.
Sorry, I meant record_track_stats
Python SDK
What did you accomplish?
How do we test the changes introduced in this PR?
Extra Notes